Update to Silverstripe 6 - #20
Conversation
NightJar
left a comment
NightJar
left a comment
There was a problem hiding this comment.
Mostly comments of curiosity - but one bug found :)
Looks good, thanks for this!
I haven't had time to look into CMS 6 yet, so this is a great way to get familiar with some of the changes (e.g. removal of Controller::has_curr) - I appreciate the input, sorry it's taken a bit longer than thought to get to its review.
| if ($relationName && ($parent = $owner->$relationName()) && $parent->hasMethod('Link')) { | ||
| $link = $parent->Link($action); | ||
| } elseif (Controller::has_curr()) { | ||
| } elseif (Controller::curr() === null) { |
There was a problem hiding this comment.
This is the exact opposite of what we want 😉
The next statement is $link = (null)->Link($action) in this case.
Since has_curr was deleted I guess it should be !== instead.
It could use assignment as to not call the function twice, but I'll leave it for you to decide.
| $count = 1; | ||
| while ($collisionList->filter($filter)->exists()) { | ||
| $owner->URLSlug = $owner->URLSlug . $count++; | ||
| $owner->URLSlug .= $count++; |
There was a problem hiding this comment.
Unrelated because this is just syntactic change, but it occurs to me we might end up with slug12345 instead of slug5 😂 - should probably write a test at some point (no need to do it for this PR though).
| /** | ||
| * Fetches the slugged item at the end of a route (slime trail). | ||
| * | ||
| * @return DataObject |
There was a problem hiding this comment.
Since the use statement was removed we should either:
- make this line fully qualified
- reinstate the
usestatement so it resolves
| 'Index ok' . | ||
| '<% end_if %>'; | ||
| return SSViewer::fromString($testTemplate); | ||
| return parent::getViewer($action); |
There was a problem hiding this comment.
I would prefer to keep test resources (i.e. templates in this case) separate from functional ones. Since there's no easy way to "set a tests template folder", inlining the template as a string seems like the best option.
This also removes the ability to override the template and make the test fail (not that anyone should override "Tests" namespace templates, but no option to is more reliable - in testing we want reliability).
| private static $has_many = [ | ||
| 'Articles' => Article::class, | ||
| 'Journalists' => Journalist::class, | ||
| 'Journalists' => Journalist::class . '.NewsPages', |
There was a problem hiding this comment.
This change seems redundant, there aren't multiple relation declarations to mix up with.
Is it a requirement in CMS 6, a personal preference, or an automation?
There was a problem hiding this comment.
Well, there's a RelationValidationService complaining, when you don't have it set correctly. See https://www.s2-hub.com/articles/validate-orm-relations-in-silverstripe/
IMHO being more declarative here is a good thing, as it makes the dev think more about relations and might prevent errors in the future.
| $this->assertEquals(404, $output->getStatusCode()); | ||
| $output = $this->get('news/contributors/jimbo-the-journo'); | ||
| $this->assertEquals('Jimbo the Journo', $output->getBody()); | ||
| $this->assertEquals('Jimbo the Journo', trim((string) $output->getBody())); |
There was a problem hiding this comment.
Was there an issue with the output? The trim call should be unnecessary; I'd rather not mutate the output under assertion to make a test pass - it should pass without interference.
Perhaps I misunderstand and the behaviour of getBody has changed?
Same for the other 2 calls above.
| 'class' => Slug::class, | ||
| 'constructor' => ['Name', 'NewsPages'], | ||
| $config->set(Journalist::class, 'extensions', [ | ||
| Slug::class . '("Name")' |
There was a problem hiding this comment.
The change also neglects to carry over the second parameter to the constructor.
Am I correct in recalling that the test didn't pass previously? 😬
I think I originally wrote it at the same time as silverstripe/silverstripe-framework#8444
There was a problem hiding this comment.
afair the second param didn't pass before, so it was useless.
| @@ -1,5 +1,6 @@ | |||
| Nightjar\Slug\Tests\Stubs\NewsPage: | |||
| holder: | |||
| Title: News Holder | |||
There was a problem hiding this comment.
Is this just a "remove confusion" change? Or something CMS 6 demands of its fixtures?
There was a problem hiding this comment.
I think without any param, the fixture wasn't generated. Might have worked before, but even back then I'd consider that unclear / not clean
2 participants
Upgrade to SS6 and PHP8.3; fixed errors in tests and complaints by my IDE's code analysis tools.
Fixes #19